Shaders refactor #6856

jfirebaugh · 2016-10-30T02:28:27Z

Followup to #6468, specifically #6468 (comment).

Fix overdraw debug mode

mention-bot · 2016-10-30T02:28:28Z

@jfirebaugh, thanks for your PR! By analyzing the history of the files in this pull request, we identified @kkaefer and @brunoabinader to be potential reviewers.

brunoabinader · 2016-10-30T09:23:29Z

Potentially fixes the issues mentioned in #6799 (comment) (needs verification).

jfirebaugh · 2016-11-01T00:00:22Z

Ready for review!

jfirebaugh · 2016-11-02T17:58:54Z

@kkaefer The test sources now need the path for generated shader .hpp's in their include paths. Where in the cmake files do I change that?

kkaefer · 2016-11-03T11:56:25Z

You can add ${MBGL_GENERATED}/include to the include dir list. The actual code that generates the shaders is in cmake/shaders.cmake.

kkaefer · 2016-11-04T10:26:44Z

I'd propose adding this to enforce initialization of the primitive types with a value:

diff --git a/src/mbgl/gl/draw_mode.hpp b/src/mbgl/gl/draw_mode.hpp
index 8f7db0a..b099a5f 100644
--- a/src/mbgl/gl/draw_mode.hpp
+++ b/src/mbgl/gl/draw_mode.hpp
@@ -11,6 +11,7 @@ public:
     using Primitive = Point;
     static constexpr std::size_t bufferGroupSize = 1;

+    Points(float pointSize_) : pointSize(pointSize_) {}
     float pointSize;
 };

@@ -19,6 +20,9 @@ public:
     using Primitive = Line;
     static constexpr std::size_t bufferGroupSize = 2;

+    Lines(float lineWidth_) : lineWidth(lineWidth_) {
+        assert(lineWidth > 0);
+    }
     float lineWidth;
 };

@@ -29,6 +33,9 @@ public:
     using Primitive = Line;
     static constexpr std::size_t bufferGroupSize = 1;

+    LineStrip(float lineWidth_) : lineWidth(lineWidth_) {
+        assert(lineWidth > 0);
+    }
     float lineWidth;
 };

kkaefer · 2016-11-04T09:44:23Z

src/mbgl/util/indexed_tuple.hpp

+    }
+};
+
+} // namespace mgbl


What problem does the indexed access solve here? It seems like the code worked before without this as well?

Less verbose -- std::get<typename As::SomeInnerType>(tuple) becomes tuple.template get<As>().

Eliminates the need to use a separate helper and std::index_sequence in Attributes::binder.

Will be consistent with use in [core] Properties refactor #6868.

kkaefer · 2016-11-04T09:45:37Z

src/mbgl/gl/segment.hpp


    std::size_t vertexLength;
-    std::size_t primitiveLength;
+    std::size_t indexLength;


Why switch to indexLength? You're saying "Segment` now tracks offset and length of indices, rather than primitives. This is more natural.", but I'm unsure what "natural" is supposed to mean.

Fundamentally, a vertex/index buffer is a buffer of vertices/indexes, not a buffer of triangles, or lines, or whatever. Conceptually, that's how OpenGL works, and I found it easier to reason about things when I stopped trying to think in terms of buffers of primitives.

Implementation wise, it simplifies calculations like this.

It matches the internal implementation of the new {Vertex,Index}Vector -- I think if we were to have std::vector<Triangle<Vertex>>, the packing might come out wrong. You don't want to get in a situation where there might be extra padding every third vertex. {Vertex,Index}Vector still enforces filling the vector with the right sized groups though.

kkaefer · 2016-11-04T09:50:46Z

src/mbgl/util/ignore.hpp

+//
+template <class T> void ignore(const std::initializer_list<T>&) {}
+
+} // namespace mbgl


Can we move ignore to the mbgl::util namespace to avoid confusion with local variables or mistaking it for a language keyword?

kkaefer · 2016-11-04T11:41:47Z

One more thing that I'd like to see (but that is out of scope for this PR) is to somehow move shaders to the context. Background is that for Node.js, we'd like to use a shared context, and ideally we can share the shaders as well to avoid having all of these Programs that are just duplicates.

kkaefer · 2016-11-07T12:55:46Z

src/mbgl/programs/raster_program.hpp

+            }
+        };
+    }
+};


Can we factor out the attributes stuff into its own objects to be in line with the others?

jfirebaugh · 2016-11-07T20:19:30Z

Rebasing this is getting to be a real drag. Can I get this approved and we can follow up on nits in other PRs?

brunoabinader · 2016-11-08T08:33:33Z

Thank you @jfirebaugh - confirming that overdraw debug mode is operational again on Linux:

…types * Extract `ignore` util to separate header. * `Segment` now tracks offset and length of indices, rather than primitives. This is more natural. * Introduce `VertexVector` and `IndexVector` types. These types carry information about the intended draw mode (`Triangles`, `LineStrip`, etc.), and ensure that elements are always appended in a group size appropriate for that draw mode, for both indexed and unindexed rendering. * `Program`, rather than `Drawable`, is now the unifying object for draw calls. `Program` is the best place to type check the draw call, because it is typed to carry information about the intended primitive, vertex type, attributes, and uniforms. * Use the debug shaders for debug tile rendering, like gl-js. * Fix the draw mode for background. It was drawing triangle strips with a triangles array. Surprised this didn’t cause issues. Now it’s type checked.

jfirebaugh force-pushed the shaders-refactor branch 5 times, most recently from 7facf12 to 4b8b19f Compare October 31, 2016 22:54

jfirebaugh added the ✓ ready for review label Nov 1, 2016

jfirebaugh force-pushed the shaders-refactor branch 4 times, most recently from 74b724e to 0782253 Compare November 2, 2016 17:37

jfirebaugh force-pushed the shaders-refactor branch 2 times, most recently from 8cdf2ec to 8d201ab Compare November 3, 2016 18:36

kkaefer reviewed Nov 4, 2016

View reviewed changes

jfirebaugh force-pushed the shaders-refactor branch 2 times, most recently from b7f71e7 to f41e21d Compare November 4, 2016 21:45

kkaefer reviewed Nov 7, 2016

View reviewed changes

jfirebaugh force-pushed the shaders-refactor branch from f41e21d to aa791f9 Compare November 7, 2016 18:46

jfirebaugh force-pushed the shaders-refactor branch from aa791f9 to 020260d Compare November 7, 2016 20:27

brunoabinader approved these changes Nov 8, 2016

View reviewed changes

kkaefer approved these changes Nov 8, 2016

View reviewed changes

[core] Make attribute binding more similar to uniform binding

36210fe

jfirebaugh added 5 commits November 8, 2016 08:09

[core] Introduce gl::Program template

66bdbc3

[core] Introduce and use IndexedTuple

d1e2fe5

[core] Put ignore in util namespace

52be5d4

[core] Require point size / line width for DrawMode constructors

3b38cf4

jfirebaugh force-pushed the shaders-refactor branch from 020260d to 3b38cf4 Compare November 8, 2016 16:09

jfirebaugh merged commit 3b38cf4 into master Nov 8, 2016

jfirebaugh deleted the shaders-refactor branch November 8, 2016 16:55

jfirebaugh mentioned this pull request Nov 11, 2016

[core] Properties refactor #6868

Merged

1ec5 removed the ✓ ready for review label Nov 29, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Shaders refactor #6856

Shaders refactor #6856

jfirebaugh commented Oct 30, 2016 •

edited

Loading

mention-bot commented Oct 30, 2016

brunoabinader commented Oct 30, 2016

jfirebaugh commented Nov 1, 2016

jfirebaugh commented Nov 2, 2016

kkaefer commented Nov 3, 2016

kkaefer commented Nov 4, 2016

kkaefer Nov 4, 2016

jfirebaugh Nov 4, 2016

kkaefer Nov 4, 2016

jfirebaugh Nov 4, 2016

kkaefer Nov 4, 2016

kkaefer commented Nov 4, 2016

kkaefer Nov 7, 2016

jfirebaugh commented Nov 7, 2016

brunoabinader commented Nov 8, 2016

Shaders refactor #6856

Shaders refactor #6856

Conversation

jfirebaugh commented Oct 30, 2016 • edited Loading

mention-bot commented Oct 30, 2016

brunoabinader commented Oct 30, 2016

jfirebaugh commented Nov 1, 2016

jfirebaugh commented Nov 2, 2016

kkaefer commented Nov 3, 2016

kkaefer commented Nov 4, 2016

kkaefer Nov 4, 2016

Choose a reason for hiding this comment

jfirebaugh Nov 4, 2016

Choose a reason for hiding this comment

kkaefer Nov 4, 2016

Choose a reason for hiding this comment

jfirebaugh Nov 4, 2016

Choose a reason for hiding this comment

kkaefer Nov 4, 2016

Choose a reason for hiding this comment

kkaefer commented Nov 4, 2016

kkaefer Nov 7, 2016

Choose a reason for hiding this comment

jfirebaugh commented Nov 7, 2016

brunoabinader commented Nov 8, 2016

jfirebaugh commented Oct 30, 2016 •

edited

Loading